-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
newruntime: implement reset #11206
newruntime: implement reset #11206
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first glance this seems great but everytime we need mixin for a generic proc, I feel like we're opening a can of worms. See comment for details and link to test cases I feel are needed.
## Destructor is invoked if T type has destructor. | ||
mixin `=destroy` | ||
`=destroy`(obj) | ||
zeroMem(obj.addr, sizeof(obj)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add tests on non-seq generic objects in a generic proc and also normal object in a block expression.
Requiring mixin here might require every proc that would use reset
to also have a mixin =destroy
.
See:
- List comprehensions do not work with generic parameter #5707 list comprehensions requires mixin with generics
- Generics proc + macros: identifier resolution happens before macros #6387 Custom seq requires mixin (but iirc seq doesn't)
- mixin + block expression: works in Generics, not in "normal" proc #7385 Mixin in a block expression only works for generic object.
Meta-issue: #8677 (Generics early symbol resolution)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requiring mixin here might require every proc that would use reset to also have a mixin =destroy.
That's not how mixin
works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In generics case it sometimes just does not work, here is another example i'm struggling with: status-im/nim-serialization#4
And the problematic type is a mixin in the template: https://github.com/status-im/nim-serialization/blob/6804ea25372de5919ef87cf13c7544770aacba2f/serialization.nim#L73
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think mixin comments are relevant . =destroy
invocation for generic type needs a mixin. Period. There is nothing reset
specific here.
I have extended the test to cover a type with no destructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I'm saying is that if we have symbol visibility rules that requires mixin for this proc, we may have symbol visibility issues where the mixin within reset
doesn't work and the package author will have to mixin =destroy
in his own proc.
It still needs to be a magic for the VM, I think, otherwise a nice solution. |
I simply mapped it to |
reset proc no longer needs to be magic